Make `AssocItem` aware of its impl kind #145186

camsteffen · 2025-08-09T21:09:19Z

The general goal is to have fewer query dependencies by making AssocItem aware of its parent impl kind (inherent vs. trait) without having to query the parent def_kind.

Introduce enum hir::ImplItemImplKind::{Inherent, Trait} at ImplItem.impl_kind which allows ImplItem to be aware of its containing impl kind, and also move impl-kind-specific fields into corresponding variants.
Split AssocItemContainer::Impl into AssocItemContainer::{InherentImpl, TraitImpl}.
Use AssocItemContainer in numerous places.

rustbot · 2025-08-09T21:09:24Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-08-09T21:09:27Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_sanitizers

cc @rcvalle

changes to the core type system

cc @compiler-errors, @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

This PR changes rustc_public

cc @oli-obk, @celinval, @ouz-a

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

camsteffen · 2025-08-10T00:59:08Z

src/librustdoc/clean/mod.rs

@@ -1258,7 +1258,10 @@ pub(crate) fn clean_impl_item<'tcx>(
            })),
            hir::ImplItemKind::Fn(ref sig, body) => {
                let m = clean_function(cx, sig, impl_.generics, ParamsSrc::Body(body));
-                let defaultness = cx.tcx.defaultness(impl_.owner_id);
+                let defaultness = match impl_.impl_kind {
+                    hir::ImplItemImplKind::Inherent { .. } => hir::Defaultness::Final,


Maybe this should be None?

compiler-errors · 2025-08-10T03:06:51Z

@bors2 try @rust-timer queue

Make `AssocItem` aware of its impl kind

rust-bors · 2025-08-10T03:06:55Z

⌛ Trying commit 77a42ec with merge ecc5941…

To cancel the try build, run the command @bors try cancel.

rust-bors · 2025-08-10T05:26:01Z

☀️ Try build successful (CI)
Build commit: ecc5941 (ecc59411274ca4c68dad1b5a2b144afaf3510243, parent: 41ede7bd9bfbc9dc1e46c5c7de3b26524e5c61f0)

rust-timer · 2025-08-10T06:34:56Z

Finished benchmarking commit (ecc5941): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	2.0%	[0.5%, 3.2%]	3
Regressions ❌ (secondary)	2.3%	[1.6%, 2.9%]	2
Improvements ✅ (primary)	-0.4%	[-1.7%, -0.2%]	20
Improvements ✅ (secondary)	-0.5%	[-1.1%, -0.2%]	40
All ❌✅ (primary)	-0.1%	[-1.7%, 3.2%]	23

Max RSS (memory usage)

Results (primary 1.4%, secondary -1.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.2%	[0.5%, 6.3%]	21
Regressions ❌ (secondary)	3.4%	[1.2%, 6.7%]	3
Improvements ✅ (primary)	-2.3%	[-4.2%, -1.3%]	5
Improvements ✅ (secondary)	-6.9%	[-7.0%, -6.8%]	3
All ❌✅ (primary)	1.4%	[-4.2%, 6.3%]	26

Cycles

Results (primary 3.3%, secondary 2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.3%	[3.3%, 3.3%]	1
Regressions ❌ (secondary)	2.3%	[1.6%, 3.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.3%	[3.3%, 3.3%]	1

Binary size

Results (primary 0.3%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.1%, 1.8%]	10
Regressions ❌ (secondary)	1.8%	[0.2%, 4.8%]	4
Improvements ✅ (primary)	-0.5%	[-1.6%, -0.1%]	4
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.1%]	12
All ❌✅ (primary)	0.3%	[-1.6%, 1.8%]	14

Bootstrap: 463.257s -> 466.12s (0.62%)
Artifact size: 377.39 MiB -> 375.29 MiB (-0.56%)

bors · 2025-08-10T18:59:46Z

☔ The latest upstream changes (presumably #145210) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot · 2025-08-11T00:18:03Z

compiler/rustc_middle/src/ty/mod.rs

-        self.assoc_parent(def_id).filter(|id| self.def_kind(id) == DefKind::Trait)
+        self.opt_associated_item(def_id)
+            .is_some_and(|assoc| assoc.container == AssocItemContainer::Trait)
+            .then(|| self.parent(def_id))


Fetching def_kind is cheaper than opt_associated_item, this change may contribute to the regression.

I think I am making this same tradeoff in several places in the PR though and it seems to yield positive results on average. This has one less query in the case where it returns None due to the assoc container (Before: def_kind+parent+def_kind. After: def_kind+associated_item). I do see there is an increase in executions (not just hits) of associated_item, so maybe there is a cost with executing that query on more items. I'm just not sure how to know which specific code change is causing that.

I looked at the cachegrind output and it looks like a large majority of the increased instructions are coming from llvm. I don't know what to make of that. I would have expected this PR to have no effect there. But that makes me think the regression cause is something surprising.

I could try splitting this PR into more incremental changes. Though most of the commits are co-dependent in some way.

I targeted this change in particular because :

def_kind is almost always already cached, and much cheaper to load,

parent is not a query but just a table access.
But this is just a hunch, I made no measurement

parent is not a query but just a table access

Oh! I misunderstood that. Okay, I'm going to look back over things with that in mind. I think you're right - I'll revert this change and maybe others.

def_kind is almost always already cached

Nit: not even "almost", it's always fed at DefId creation.

Okay, now with a lower aversion to parent 😅 , I reverted some changes and changed / split out some changes into #145266.

compiler/rustc_middle/src/ty/mod.rs

petrochenkov · 2025-08-11T16:47:15Z

Oh! I misunderstood that. Okay, I'm going to look back over things with that in mind. I think you're right - I'll revert this change and maybe others.

@rustbot author

rustbot · 2025-08-11T16:47:20Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

camsteffen · 2025-08-11T19:09:42Z

@rustbot ready (pending CI)

petrochenkov · 2025-08-11T19:26:42Z

@bors2 try @rust-timer queue

Make `AssocItem` aware of its impl kind

rust-bors · 2025-08-11T21:39:42Z

☀️ Try build successful (CI)
Build commit: 68df66d (68df66da903eecbd55c3d2a4c8862df210670e24, parent: fce0e74720d199eb7839fdb51af35ac5226da178)

rust-timer · 2025-08-12T03:08:25Z

Finished benchmarking commit (68df66d): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	15
Improvements ✅ (secondary)	-0.4%	[-0.7%, -0.2%]	22
All ❌✅ (primary)	-0.2%	[-0.3%, -0.2%]	15

Max RSS (memory usage)

Results (primary -0.4%, secondary -6.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.2%	[-1.6%, -0.8%]	2
Improvements ✅ (secondary)	-6.8%	[-6.9%, -6.8%]	3
All ❌✅ (primary)	-0.4%	[-1.6%, 1.3%]	3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	6
All ❌✅ (primary)	-	-	0

Bootstrap: 464.993s -> 464.709s (-0.06%)
Artifact size: 377.36 MiB -> 377.31 MiB (-0.01%)

rustbot assigned petrochenkov Aug 9, 2025

This comment has been minimized.

Sign in to view

camsteffen force-pushed the assoc-impl-kind branch from 0617ff2 to cfed1ee Compare August 9, 2025 22:31

This comment has been minimized.

Sign in to view

camsteffen force-pushed the assoc-impl-kind branch from cfed1ee to 77a42ec Compare August 10, 2025 00:30

camsteffen commented Aug 10, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Aug 10, 2025

Auto merge of #145186 - camsteffen:assoc-impl-kind, r=<try>

ecc5941

Make `AssocItem` aware of its impl kind

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 10, 2025

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 10, 2025

cjgillot reviewed Aug 11, 2025

View reviewed changes

compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2025

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 11, 2025

camsteffen added 4 commits August 11, 2025 14:00

Introduce hir::ImplItemImplKind

167df48

Split AssocItemContainer::{InherentImpl,TraitImpl}

4d28a9d

Don't store defaultness for inherent impl items

27742c8

Use AssocItemContainer more

1d8ed9f

camsteffen force-pushed the assoc-impl-kind branch from 77a42ec to 1d8ed9f Compare August 11, 2025 19:02

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 11, 2025

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Aug 11, 2025

Auto merge of #145186 - camsteffen:assoc-impl-kind, r=<try>

68df66d

Make `AssocItem` aware of its impl kind

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 11, 2025

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2025

camsteffen mentioned this pull request Aug 11, 2025

Limit impl_trait_header query to only trait impls #144607

Open

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Aug 12, 2025

Make AssocItem aware of its impl kind #145186

Are you sure you want to change the base?

Make AssocItem aware of its impl kind #145186

Conversation

camsteffen commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 9, 2025

Uh oh!

rustbot commented Aug 9, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Aug 10, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Aug 10, 2025

Uh oh!

rust-bors bot commented Aug 10, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Aug 10, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

bors commented Aug 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camsteffen Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petrochenkov commented Aug 11, 2025

Uh oh!

rustbot commented Aug 11, 2025

Uh oh!

camsteffen commented Aug 11, 2025

Uh oh!

petrochenkov commented Aug 11, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Aug 11, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Aug 12, 2025

Overall result: ✅ improvements - no action needed

Uh oh!

Uh oh!

Make `AssocItem` aware of its impl kind #145186

Make `AssocItem` aware of its impl kind #145186

camsteffen commented Aug 9, 2025 •

edited

Loading

camsteffen Aug 11, 2025 •

edited

Loading